Add deterministic SVG degradation curve renderer#128
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for comptext-v7 canceled.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a deterministic SVG renderer for visualizing layered admissibility degradation curves, along with a supporting CLI script and comprehensive tests. The implementation focuses on reproducibility by using fixed canvas geometry and stable float precision. Feedback was provided to improve the robustness of the renderer, including handling missing input data to avoid KeyErrors, preventing potential division-by-zero errors in coordinate calculations, and ensuring SVG background dimensions are dynamically linked to class constants. Additionally, it was suggested to include all failure labels in the visualization, even those not explicitly listed in the predefined annotation order.
|
|
||
| layouts: list[_PointLayout] = [] | ||
| for index, (fixture_id, _) in enumerate(self.X_TICKS): | ||
| point = points_by_fixture[fixture_id] |
There was a problem hiding this comment.
Accessing points_by_fixture[fixture_id] directly will raise a KeyError if the input JSON is missing any of the expected benchmark fixtures defined in X_TICKS. Adding a check or a more descriptive error message would improve the robustness of the renderer, especially when used in CI environments.
| point = points_by_fixture[fixture_id] | |
| if fixture_id not in points_by_fixture: | |
| raise KeyError(f"Fixture '{fixture_id}' not found in input data.") | |
| point = points_by_fixture[fixture_id] |
| for index, (fixture_id, _) in enumerate(self.X_TICKS): | ||
| point = points_by_fixture[fixture_id] | ||
| score = float(point["overall_admissibility_score"]) | ||
| x = self.MARGIN_LEFT + (plot_width * index / (len(self.X_TICKS) - 1)) |
There was a problem hiding this comment.
This calculation will result in a ZeroDivisionError if X_TICKS is modified to contain only one element. Adding a guard for the denominator ensures the renderer remains robust against changes to the tick configuration.
| x = self.MARGIN_LEFT + (plot_width * index / (len(self.X_TICKS) - 1)) | |
| x = self.MARGIN_LEFT + (plot_width * index / (len(self.X_TICKS) - 1)) if len(self.X_TICKS) > 1 else self.MARGIN_LEFT |
| polyline_points = " ".join(f"{self._fmt(p.x)},{self._fmt(p.y)}" for p in layouts) | ||
| elements: list[str] = [ | ||
| f'<svg xmlns="http://www.w3.org/2000/svg" width="{self.WIDTH}" height="{self.HEIGHT}" viewBox="0 0 {self.WIDTH} {self.HEIGHT}">', | ||
| ' <rect x="0" y="0" width="1000" height="520" fill="#ffffff"/>', |
There was a problem hiding this comment.
The background rectangle uses hardcoded width and height values (1000 and 520) instead of referencing the class constants WIDTH and HEIGHT. This will cause the background to be incorrectly sized if the canvas dimensions are updated in the future.
| ' <rect x="0" y="0" width="1000" height="520" fill="#ffffff"/>', | |
| f' <rect x="0" y="0" width="{self.WIDTH}" height="{self.HEIGHT}" fill="#ffffff"/>', |
| ordered_labels = [label for label in self.FAILURE_ANNOTATION_ORDER if label in point.failure_labels] | ||
| if ordered_labels: | ||
| elements.append( | ||
| f' <text x="{self._fmt(point.x)}" y="{y_base}" text-anchor="middle" font-size="10" font-family="monospace" fill="#aa2200">{", ".join(ordered_labels)}</text>' | ||
| ) |
There was a problem hiding this comment.
Failure labels that are not explicitly listed in FAILURE_ANNOTATION_ORDER are silently omitted from the visualization. To ensure all degradation information is surfaced, consider appending any unexpected labels to the end of the annotation list.
| ordered_labels = [label for label in self.FAILURE_ANNOTATION_ORDER if label in point.failure_labels] | |
| if ordered_labels: | |
| elements.append( | |
| f' <text x="{self._fmt(point.x)}" y="{y_base}" text-anchor="middle" font-size="10" font-family="monospace" fill="#aa2200">{", ".join(ordered_labels)}</text>' | |
| ) | |
| # Prioritize labels in FAILURE_ANNOTATION_ORDER, then include any others | |
| labels = [l for l in self.FAILURE_ANNOTATION_ORDER if l in point.failure_labels] | |
| labels.extend(sorted(set(point.failure_labels) - set(self.FAILURE_ANNOTATION_ORDER))) | |
| if labels: | |
| elements.append( | |
| f' <text x="{self._fmt(point.x)}" y="{y_base}" text-anchor="middle" font-size="10" font-family="monospace" fill="#aa2200">{", ".join(labels)}</text>' | |
| ) |
1 participant
Motivation
artifacts/layered_admissibility_results.jsonto a stable SVG artifact.Description
SVGCurveRenderer(src/visualization/svg_curve_renderer.py) that maps fixture progression (positive → mild → moderate → severe) to deterministic SVG coordinates with fixed canvas (1000×520), margins, stable ordering, and three-decimal float formatting. Changed files:src/visualization/svg_curve_renderer.py,src/visualization/__init__.py.scripts/render_curve_svg.pyto regeneratedocs/media/layered_admissibility_curve.svgfrom the committed JSON in a CI-friendly, importable way.tests/test_svg_curve_renderer.py) that assert root SVG validity, fixture labels, presence and ordering of the expected failure annotations, monotonic plotted coordinates, stable float formatting, and exact parity with the committed SVG artifact.docs/media/layered_admissibility_curve.svganddocs/benchmarks/layered_admissibility.mdnow include a Visualization section referencing the deterministic SVG.Testing
pytest tests/test_svg_curve_renderer.py -q— passed (7 tests).pytest -q— passed (184 passed).npm run check— passed (layout/typecheck/validate/build/test pipeline completed).Codex Task